-
Notifications
You must be signed in to change notification settings - Fork 219
Conversation
The release ZIP for this PR is accessible via:
|
TypeScript Errors ReportFiles with errors: 427 🎉 🎉 This PR does not introduce new TS errors. |
Size Change: -360 B (0%) Total Size: 972 kB
ℹ️ View Unchanged
|
@alexflorisca I know this PR is still in draft, but I took a brief look and noticed two things:
|
Thanks for your comment Niels! So I've made some changes and you can officially review it! I added your suggestion and the user now can't save an empty button. It will default to I've also translated the default text for the button, so hopefully that should work now, but for some reason I can't get anything to translate on my local test side. Could use your help here. There's also another bug with the Let me know what you think, |
Thanks for applying the changes, @alexflorisca!
Nice! This change works as expected both for English and non-English strings. On my end, I tested it both for
Happy to jump on a call with you to sort this out. Sometimes translations act a bit strange. What I do, occasionally, is resetting the translations. You can do that as follows:
This will reset all translations, and you can run a fresh installation of the desired language via
Sounds good. I'd probably add the issue number to this ticket in case we're looking up this ticket in the future. That said, I'd not wait for the upstream fix, but go ahead and deploy this PR already. I assume once this issue got addressed in upstream, we'll automatically receive the change and there's nothing for us to adjust, given that you're already calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @alexflorisca! 🙌 I'm pre-approving this PR as it works as expected, incl. the translations. I just left a minor suggestion regarding where to store the default button label.
export const defaultButtonLabel = __( | ||
'Proceed to Checkout', | ||
'woo-gutenberg-products-block' | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexflorisca I wonder if we should move the button label into a designated file, e.g. constants.tsx
. We're doing the same for the product elements currently:
woocommerce-blocks/assets/js/atomic/blocks/product-elements/image/constants.tsx
Lines 7 to 17 in 8c857e2
export const BLOCK_TITLE: string = __( | |
'Product Image', | |
'woo-gutenberg-products-block' | |
); | |
export const BLOCK_ICON: JSX.Element = ( | |
<Icon icon={ image } className="wc-block-editor-components-block-icon" /> | |
); | |
export const BLOCK_DESCRIPTION: string = __( | |
'Display the main product image.', | |
'woo-gutenberg-products-block' | |
); |
That way, we keep block.tsx
lean, and we can still use the translatable string both in block.tsx
and in attributes.tsx
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only move things to constants file when they're dynamic, this string should be in attributes.jsx or block.json
onChange={ ( content ) => { | ||
if ( content === '' ) { | ||
setAttributes( { | ||
buttonLabel: defaultButtonLabel, | ||
} ); | ||
} else { | ||
setAttributes( { | ||
buttonLabel: content, | ||
} ); | ||
} | ||
} } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this effort, why are we setting the attribute back to its default value? I understand that you're trying to protect against people having zero value but couldn't this be achieved using the built in attribute handling in Gutenberg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I tried initially, but if you have an empty string, and save the attribute, it will save it with the empty string, not the default value. So this counters that and doesn't let you save the attribute if it's empty.
…rce/woocommerce-blocks into edit-proceed-to-checkout-button
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
We're waiting on the outcome of this discussion (p1670237870752419/1669983170.345659-slack-C8X6Q7XQU) to merge |
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
Based on the discussion here (pdToLP-f7-p2#comment-163), I'm leaving this as is and merging. |
This PR adds the ability to edit the
Proceed to Checkout
button on the Cart block.Challenges to solve:
This was due to the default attribute not being a translatable string. I've now added this in, but for some reason I can't get ANYTHING to translate. @nielslange maybe you can help me out here a little?
We're not REALLY duplicating the button, we are just wrapping the
RichText
component in a<Button className="wc-block-cart__submit-button">
to give it the right styling. I think this is ok, doesn't duplicate any of the props passed to the button inblock.tsx
and simplifies the code by not having to wrap the component in a<NonInteractive>
component<br />
in the front endThis seems to be a bug with the
RichText
editor, as themultiline
prop is supposed to restrict input to one line only. Will investigate this with Gutenberg.Fixes #7012
Accessibility
prefers-reduced-motion
Other Checks
Testing
Automated Tests
User Facing Testing
Proceed to Checkout
button. You should be able to edit the text of this buttonWooCommerce Visibility
Performance Impact
Changelog